Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RCAL-905: Add ePSF, ABVegaOffset, and ApCorr Datamodels #393

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

RCAL-905: Add ePSF, ABVegaOffset, and ApCorr Datamodels #393

wants to merge 6 commits into from
+229 −11

Conversation

PaulHuwe
Copy link
Collaborator

@PaulHuwe PaulHuwe commented Sep 30, 2024 •
edited
Loading

Resolves RCAL-905

Closes #905

This PR adds datamodels and tests for ePSF, ABVegaOffset, and ApCorr reference files. Other tests were tweaked for this support. The PR template was also updated to match the one from the RAD library.

Tasks

  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • Does this PR change any API used downstream? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • start a romancal regression test with this branch installed ("git+https://github.com/<fork>/roman_datamodels@<branch>")
news fragment change types...
  • changes/<PR#>.feature.rst: new feature
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.doc.rst: documentation change
  • changes/<PR#>.removal.rst: deprecation or removal of public API
  • changes/<PR#>.misc.rst: infrastructure or miscellaneous change

@codecov Codecov
Copy link

codecov bot commented Sep 30, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 97.87234% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.75%. Comparing base (087a60d) to head (7a58f89).
Report is 51 commits behind head on main.

Files with missing lines Patch % Lines
src/roman_datamodels/maker_utils/_ref_files.py 95.45% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #393      +/-   ##
==========================================
+ Coverage   97.56%   97.75%   +0.19%     
==========================================
  Files          30       37       +7     
  Lines        2788     3471     +683     
==========================================
+ Hits         2720     3393     +673     
- Misses         68       78      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@PaulHuwe
Copy link
Collaborator Author

RCAL regression tests: https://github.com/spacetelescope/RegressionTests/actions/runs/11097495928

@stscijgbot-rstdms stscijgbot-rstdms mentioned this pull request Sep 30, 2024
Open
@PaulHuwe PaulHuwe marked this pull request as ready for review September 30, 2024 15:51
@PaulHuwe PaulHuwe requested a review from a team as a code owner September 30, 2024 15:51
schlafly
schlafly reviewed Sep 30, 2024
View reviewed changes
src/roman_datamodels/maker_utils/_ref_files.py
epsfref["meta"] = mk_ref_epsf_meta(**kwargs.get("meta", {}))

epsfref["psf"] = kwargs.get("psf", np.zeros(shape, dtype=np.float32))
epsfref["extended_psf"] = kwargs.get("extended_psf", np.zeros(shape[2:], dtype=np.float32))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left most of the relevant comments in the rad PR; this all looks good. But one dumb comment here: in the actual files, is the extended_psf shape actually the same as in the psf stamps, or significantly larger? I hope larger, even if we don't do that for the dummy epsf objects?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one sample file I have does not include the extended_psf (it is an optional array keyword). By default, the maker utility just uses a dummy shape and is not representative of reality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schlafly schlafly schlafly left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PaulHuwe @schlafly